-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
contrib: add gorm.io/gorm #759
Conversation
…nately must be passed as an additional parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend taking a look at other integrations to figure out what the style conventions and recommendations are for naming etc.
Once it's ready for review, just take it out of draft.
Thank you for your comments. I will first make sure all tests pass and then re-order everything to be according to your coding conventions |
I'm pretty sure this is in accordance to your guidelines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the work. You'll need to bear with us for a few more rounds of reviews but I think we're on the right path.
As for the CI test, no worries, we will look at that separately. It does seem unrelated to your PR. However, you will have to use gofmt
on your code.
CI is fixed. If you still plan on continuing the PR, once you rebase on |
Thanks! |
Sorry, I've only now had a chance to take a look at this. Previously (in #714) I had looked into making a
@AdallomRoy What do you think about a Plugin? Do we lose anything by not using the Dialector interface? |
Nice! I can check this out later this week and let you know |
Fixes #684